@neolynx here is another annoying thing I'd like to fix in aptly. My publish prefix is bloated with empty directories as I have to create and remove publishes intensively.

oh, yes please ! I found this quite annoying as well...

yay, there is an issue for it! #198

@neolynx original tests that were failing are now fixed, but my newly added PublishDrop21Test is failing, not sure why...

May be it's time for me to learn something new, too? 😁 This alienish test suite is driving me nuts, frankly...

All tests are passing now

@neolynx original tests that were failing are now fixed

great ! could you explain why they failed ?

but my newly added PublishDrop21Test is failing, not sure why...

May be it's time for me to learn something new, too? 😁 This alienish test suite is driving me nuts, frankly...

please do not judge :) the history of all of this is in git, many people worked on that. I think the thing to learn here is how to deal with legacy code and how to slowly improve it and revive the project. so please be respectful and constructive, we are all aware of the state of things, and it took months to get it to a working and halfway reliable state.

could you explain why they failed ?

-gold file was missing for my PublishDrop21Test - copied it from PublishDrop2Test. Not completely sure what it is.

Whole experience of adding tests is cumbersome and non-obvious due to lack of developers documentation. Even some doc links to the testing frameworks would make the whole process more friendly and smooth...

Oh, and original tests were failing because checks relied on empty directories now removed (as you said, the history of all of this is in git :-) )

could you explain why they failed ?

-gold file was missing for my PublishDrop21Test - copied it from PublishDrop2Test. Not completely sure what it is.

ok great. in the Makefile there is the CAPTURE flag, you can uncomment, then the tests will create the gold files.

Whole experience of adding tests is cumbersome and non-obvious due to lack of developers documentation. Even some doc links to the testing frameworks would make the whole process more friendly and smooth...

as said before, we are well aware of that. feel free to contribute. until then let's keep the discussions here technical.

Oh, and original tests were failing because checks relied on empty directories now removed (as you said, the history of all of this is in git :-) )

thanks, that is excellent :-)

tests look good !

I started a coverage build: codecov/patch — 84.61% of diff hit (target 74.84%)

Seems, ci is stuck again...

Seems, ci is stuck again...

seems we have a segfault:
https://github.com/aptly-dev/aptly/actions/runs/11476603052/job/31936962068?pr=1381#step:8:487

seems we have a segfault

Heh, that's because nil is passed instead of aptly.Progress in tests. Very handy 😒

P.S.: handled that on my side

Now, seems -gold-en outputs do not match new reality 😂

Now, seems -gold-en outputs do not match new reality 😂

it needs that non empty check before remove... ;-)

Also, something wrong with docker tests:

$ make docker-image
[+] Building 310.3s (21/21) FINISHED
$ make docker-system-test
usermod: UID '0' already exists
make: *** [docker-system-test] Error 4
$ _

So, for me there is no simple way to update test now, unfortunately.

it needs that non empty check before remove... ;-)

You know what, my experience contributing in this particular PR is drastically falling down. Second day in a row we're chewing through tons of comments in a changeset that barely changes nothing, docs on testsuite are missing, tooling does not work and you're making jokes on me...

That all really makes me sad.

Also, something wrong with docker tests:

the docker-wrapper sets the user to the one of the git repo directory, to not write files as root. it seems your git dir is owned by root. changing to user owner would solve this in that case.

it seems your git dir is owned by root

not true

$ ls -lad ../aptly/
drwxr-xr-x@ 46 aol  staff  1472 Oct 22 17:22 ../aptly/

Thanks for the docker-wrapper idea, though. Slashed it a bit.

Re: better tooling #1384

It might be worth merging it first.

A friendly ping for @neolynx

Tests are fixed now.

Let's finish those two PRs, we're on the homestretch ;)

Rebased to the tip of master, conflicts resolved.

P.S.: Failing tests are not mine.

P.S.: Failing tests are not mine.

resources from the internet are not always available, I restarted the tests.

Dear @aol-nnov,

I was by no means making jokes on you, i was merely a wink to point out what is expected here. Your skills and contributions are very welcome, also for documentation and all the other stuff which is lagging behind !

I talked to some of the other maintainers, here are our thougths:

• Aptly does not have the authority over the prefix path, as it is most likely used in a webserver directory structure, which might be managed by other tools
• Changing the default behavior to remove the tree could result in 404 instead of a empty directory index on the webserver
• Proper logging of unexpected error is required, as aptly logs are crucial for reproducing issues, also expected errors are prevented / not logged
• there is also deb/publish.go:func (p *PublishedRepo) RemoveFiles(publishedStorageProvider aptly.PublishedStorageProvider, removePrefix bool, - the removePrefix might be doing something similar, maybe this can be combined?
• Issue comments are marked as resolved by the maintainers. Even if the concern might feel unreasonable or not to your liking.

To move forward here, we suggest the following changes:

• make a cleanupTree argument to the drop calls (api, cmd), cleanup up the tree only if set to true and skipCleanup is false
• in CleanupPublishTree, check if the directory is empty and return if true, otherwise remove directory. return the error and log in the caller
• original test should prove that prefix is not removed, new tests prove that empty dirs are removed, non-empty ones not (without a error log)
• if we wanna be fancy, a test with a parent directory without write permissions would prove that the error is logged

Please understand that aptly is a legacy project with many maintainers and contributors over the years. It took considerable effort to revive it and bring it to the state it is now. As maintainers, our main concern is to not break any of potentially thousands of installations, and if something happens, we have proper logs. And this has some implications on how we can implement things.

@cfiehe, @szakalboss, @dario-gallucci please add if I forgot something

Dear @neolynx,

First of all, I'd like to point that all stated below is not personal offence, but a feedback for the project maintainer handling incoming changes.

Overall experience during contributing to this miserly PR was cumbersome and disappointing. Starting from constantly changing and somewhat controversial requirements (I'm not even asking now how would checking directory for the emptiness before removal will aid 404s on missing dirs and such and such) up to never-ending discussions which made me feel totally not understood, and to suddenly-disappearing-reviewer right in the middle of threads without any notice on coming-back-or-not and many other obstacles along the way...

As we know, all good things eventually come to their end. So do my resources, that I'm able to invest into this PR.

Feel free to implement this change exactly "the way you feel", as all technical reasons I brought to you were discarded or ignored by you along the way. Also feel free to take the credits for this change - I do not mind.

Our enterprise is heavily using aptly in production for over a decade now (and contributing to it, as well), and we were forced to operate on our fork for a few times already, rather than upstrean version due to abandonedness or the project, beurocracy of accepting patches or mantainer's irresponsiveness.

Sadly, it seems, we're on the fork again. We need to mind our business, not to prove something on the Internet.

Good luck and thanks for all the fish. (Making it clear: I'm no longer able to work on this PR.)